feat: add CloudEvents webhook support for AWS ECR #1370
Conversation
audacioustux
requested review from
chengfang,
dkarpele,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds CloudEvents webhook support: new Changes
sequenceDiagram
autonumber
participant ECR as AWS ECR
participant EB as EventBridge
participant API as API Destination
participant Webhook as Webhook Server
participant Parser as CloudEventsWebhook
ECR->>EB: emit image.push CloudEvent
EB->>API: rule matches → invoke API Destination
API->>Webhook: POST CloudEvents JSON (body + header or ?secret)
Webhook->>Webhook: validate method & Content-Type
Webhook->>Webhook: verify secret (header or query) if configured
Webhook->>Parser: forward request body
Parser->>Parser: parse CloudEvent (specversion,type,source,subject,data)
Parser->>Webhook: return WebhookEvent {RegistryURL, Repository, Tag, Digest}
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code graph analysis (2)cmd/run_test.go (1)
cmd/run.go (1)
🪛 LanguageTooldocs/install/cmd/run.md[style] ~40-~40: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE) 🪛 markdownlint-cli2 (0.18.1)docs/configuration/webhook.md103-103: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1370 +/- ##
==========================================
+ Coverage 70.71% 71.43% +0.71%
==========================================
Files 49 50 +1
Lines 4538 4673 +135
==========================================
+ Hits 3209 3338 +129
- Misses 1132 1135 +3
- Partials 197 200 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
6017d44 to
f66440c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/webhook/cloudevents_test.go (1)
124-136: Consider adding test case for tag containing colon.The subject parsing logic at line 131-134 in cloudevents.go uses
strings.Split(payload.Subject, ":")withlen(parts) == 2. Tags likesha256:abc123or timestamps like2025-01-01:latestwould not parse correctly from the subject. Consider adding a test case to document this behavior or verify the data fields take precedence.Example test case to add:
{ name: "ECR event with colon in tag uses data fields", payload: map[string]interface{}{ "specversion": "1.0", "id": "test-id", "type": "com.amazon.ecr.image.push", "source": "urn:aws:ecr:us-east-1:123456789012:repository/myrepo", "subject": "myrepo:v1:latest", // malformed subject "time": "2025-11-27T10:00:00Z", "datacontenttype": "application/json", "data": map[string]interface{}{ "repositoryName": "myrepo", "imageDigest": "sha256:abc", "imageTag": "v1:latest", "registryId": "123456789012", }, }, wantRegistryURL: "123456789012.dkr.ecr.us-east-1.amazonaws.com", wantRepository: "myrepo", wantTag: "v1:latest", wantDigest: "sha256:abc", wantErr: false, },pkg/webhook/cloudevents.go (2)
130-135: Subject parsing is fragile for tags containing colons.The split on
:withlen(parts) == 2check will fail for tags containing colons (e.g.,repo:v1:hotfixor timestamps). Since data fields override subject anyway, consider usingstrings.SplitNwith a limit of 2 to handle this edge case gracefully.- if parts := strings.Split(payload.Subject, ":"); len(parts) == 2 { - repository = parts[0] - tag = parts[1] + if parts := strings.SplitN(payload.Subject, ":", 2); len(parts) == 2 { + repository = parts[0] + tag = parts[1] logCtx.Tracef("Extracted from subject: repository=%s, tag=%s", repository, tag) }
227-252: Simplify extractECRRegistryURL by removing duplicate parsing.The fallback block at lines 239-248 re-parses the source even though the primary block at lines 229-236 already extracts both region and account. If the primary parsing succeeds, the fallback is never needed. If it fails, the fallback with
registryIdstill requires a valid source prefix to get the region, making it partially redundant.func (c *CloudEventsWebhook) extractECRRegistryURL(source string, data map[string]interface{}) string { // Try to extract from source URN if strings.HasPrefix(source, "urn:aws:ecr:") { parts := strings.Split(source, ":") if len(parts) >= 5 { region := parts[3] account := parts[4] + // Use registryId from data if available, otherwise use account from source + if registryID, ok := data["registryId"].(string); ok && registryID != "" { + account = registryID + } return fmt.Sprintf("%s.dkr.ecr.%s.amazonaws.com", account, region) } } - - // Fallback: try to extract from data - if registryID, ok := data["registryId"].(string); ok { - // If we have registryId, we need region to construct the URL - // Try to extract region from source - if strings.HasPrefix(source, "urn:aws:ecr:") { - parts := strings.Split(source, ":") - if len(parts) >= 4 { - region := parts[3] - return fmt.Sprintf("%s.dkr.ecr.%s.amazonaws.com", registryID, region) - } - } - } - return "" }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/common.go(2 hunks)cmd/run.go(1 hunks)cmd/webhook.go(2 hunks)config/examples/cloudevents/README.md(1 hunks)config/examples/cloudevents/terraform/main.tf(1 hunks)config/examples/cloudevents/terraform/outputs.tf(1 hunks)config/examples/cloudevents/terraform/variables.tf(1 hunks)config/examples/cloudevents/test-webhook.sh(1 hunks)docs/configuration/webhook.md(5 hunks)pkg/webhook/cloudevents.go(1 hunks)pkg/webhook/cloudevents_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/run.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
cmd/common.go (1)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(22-26)
cmd/webhook.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
pkg/webhook/cloudevents_test.go (2)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(22-26)registry-scanner/pkg/registry/mocks/Repository.go (1)
Repository(15-17)
pkg/webhook/cloudevents.go (2)
registry-scanner/pkg/log/log.go (4)
NewContext(34-40)Tracef(159-162)Errorf(183-186)LogContext(26-31)pkg/argocd/types.go (1)
WebhookEvent(196-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (16)
cmd/common.go (2)
29-29: LGTM!The new
CloudEventsSecretfield follows the established pattern for other webhook secrets in the struct.
121-121: LGTM!The CloudEvents webhook handler registration follows the same pattern as existing handlers (Docker, GHCR, Harbor, Quay), maintaining consistency.
config/examples/cloudevents/README.md (1)
1-35: LGTM!Clear and well-structured documentation for the CloudEvents webhook example. The quick start guide, file listing, and documentation link provide a good onboarding experience.
cmd/run.go (1)
307-307: LGTM!The new CLI flag follows the established pattern for other webhook secrets, with consistent naming and environment variable binding.
config/examples/cloudevents/test-webhook.sh (1)
14-92: LGTM!Good test coverage with four distinct scenarios: successful ECR push, generic container push with CloudEvents content-type, missing secret validation, and invalid event type handling. The
|| echo "Expected failure"pattern correctly handles expected failures withset -e.config/examples/cloudevents/terraform/outputs.tf (1)
1-19: LGTM!Well-structured Terraform outputs with clear descriptions. The outputs expose useful ARNs for debugging and integration purposes.
cmd/webhook.go (2)
44-45: LGTM!Help text correctly updated to include Harbor and AWS ECR via EventBridge CloudEvents in the list of supported registries.
93-93: LGTM!The CLI flag binding is consistent with the
runcommand (cmd/run.go, line 307) and follows the established pattern for other webhook secrets.docs/configuration/webhook.md (2)
12-12: Documentation is clear and comprehensive.The AWS ECR via EventBridge CloudEvents section provides thorough setup instructions, prerequisites, and example payloads. The documentation follows the existing patterns for other registries.
Also applies to: 88-159
161-162: Unable to verify directory existence due to repository access issues.The sandbox environment is experiencing persistent failures when attempting to clone the repository, preventing verification of whether the
config/examples/cloudevents/kubernetes/directory exists in the PR. Manual verification of this directory in the repository is required to confirm whether the documentation reference is accurate or if the directory needs to be added.config/examples/cloudevents/terraform/main.tf (2)
1-141: Well-structured Terraform configuration following AWS best practices.The module correctly implements:
- Least-privilege IAM with scoped
InvokeApiDestinationpermission- API_KEY authentication via header
- Configurable repository filtering
- Retry policy for resilience
118-134: Based on the web search verification results, I have sufficient information to assess the review comment.The review comment is substantively correct and identifies a real issue.
The web search confirms:
- ECR does send events without
image-tagfor untagged/manifest-only pushes — the event detail will either omit the field entirely or have it as null/empty- EventBridge InputTransformer with missing source paths — when the
<tag>placeholder references a path that doesn't exist in the source event, it will either fail to substitute or produce invalid JSON- The current template is vulnerable — using
"imageTag": "<tag>"means untagged image pushes will either cause the transformer to fail or produce malformed CloudEventsThe concern about handling manifest-only ECR pushes (which lack tags) is legitimate and should be addressed through either:
- Adding filtering rules to exclude untagged images
- Making the imageTag field optional in the CloudEvents template
- Documenting this limitation
**Input template will fail or produce invalid JSON for untagged ECR images due to missing `image-tag` field.** ECR manifest-only pushes (untagged images) do not include an `image-tag` in the EventBridge event detail. The current template uses `"imageTag": "<tag>"` which will cause the InputTransformer to fail when this field is absent. Either filter these events in the rule or modify the template to handle missing tags gracefully.config/examples/cloudevents/terraform/variables.tf (1)
1-32: Variables are well-defined with appropriate types and sensitivity markers.The
webhook_secretis correctly marked assensitive = true, preventing it from appearing in logs or plan outputs.pkg/webhook/cloudevents_test.go (2)
19-122: Comprehensive validation test coverage.The test suite covers key validation scenarios including HTTP method, content types, query/header secrets, and missing/invalid secrets.
267-420: Good coverage of generic CloudEvents parsing.Tests appropriately cover alternate field names, source URL extraction, and error cases for missing specversion/type.
pkg/webhook/cloudevents.go (1)
14-31: Clean implementation following established webhook patterns.The struct design, constructor, and parsing logic are well-organized. Trace logging for debugging webhook payloads is a good addition for troubleshooting.
Also applies to: 74-115
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
2 times, most recently
from
9cc93c0 to
b3cac08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/configuration/webhook.md (1)
247-256: Fix GHCR flag name; CloudEvents env mapping looks correctThe new row mapping
CLOUDEVENTS_WEBHOOK_SECRET→--cloudevents-webhook-secretis consistent withcmd/webhook.go. However, the GHCR row above documents--gchr-webhook-secret, while the actual flag incmd/webhook.gois--ghcr-webhook-secret. Consider correcting that flag name in this table while touching this area.
🧹 Nitpick comments (2)
config/examples/cloudevents/test-webhook.sh (1)
14-90: Consider enforcing expected success/failure via curl exit codesRight now the script just prints HTTP status codes, and the “expected to fail” tests rely on curl exiting non‑zero, which won’t happen for HTTP 4xx/5xx without
--fail/--fail-with-body. To make this a more reliable e2e check, consider adding--fail-with-body(or parsing%{http_code}) for tests 1–4 and explicitly asserting on success vs failure instead of relying on manual inspection.pkg/webhook/cloudevents.go (1)
188-195: Make generic registry URL extraction robust to blank valuesIn the generic container/image path,
registryUrl/registryare treated as authoritative as long as the key exists, even if the value is an empty string, which prevents falling back toextractRegistryFromSourceand leads straight to the “registry URL not found” error. If you want to be resilient to partially-populated data, consider also checking for non-empty strings before assignment, e.g.:- if regURL, ok := payload.Data["registryUrl"].(string); ok { + if regURL, ok := payload.Data["registryUrl"].(string); ok && regURL != "" { registryURL = regURL - } else if regURL, ok := payload.Data["registry"].(string); ok { + } else if regURL, ok := payload.Data["registry"].(string); ok && regURL != "" { registryURL = regURL } else { registryURL = c.extractRegistryFromSource(payload.Source) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/common.go(2 hunks)cmd/run.go(1 hunks)cmd/webhook.go(2 hunks)config/examples/cloudevents/README.md(1 hunks)config/examples/cloudevents/terraform/main.tf(1 hunks)config/examples/cloudevents/terraform/outputs.tf(1 hunks)config/examples/cloudevents/terraform/variables.tf(1 hunks)config/examples/cloudevents/test-webhook.sh(1 hunks)docs/configuration/webhook.md(5 hunks)pkg/webhook/cloudevents.go(1 hunks)pkg/webhook/cloudevents_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/common.go
- config/examples/cloudevents/terraform/outputs.tf
- pkg/webhook/cloudevents_test.go
- config/examples/cloudevents/terraform/main.tf
- config/examples/cloudevents/terraform/variables.tf
- cmd/run.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/webhook.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
pkg/webhook/cloudevents.go (3)
registry-scanner/pkg/log/log.go (4)
NewContext(34-40)Tracef(159-162)Errorf(183-186)LogContext(26-31)pkg/argocd/types.go (1)
WebhookEvent(196-205)registry-scanner/pkg/registry/mocks/Repository.go (1)
Repository(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (go)
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure code is correctly linted
- GitHub Check: build_image
🔇 Additional comments (3)
docs/configuration/webhook.md (1)
102-164: CloudEvents/ECR documentation aligns with implementationThe new “AWS ECR via EventBridge CloudEvents” section (payload shape, webhook URL with
type=cloudevents, and secret handling via query orX-Webhook-Secret) matches the CloudEvents handler and examples. This gives users a clear, end‑to‑end path from EventBridge to the webhook.cmd/webhook.go (1)
40-46: CloudEvents webhook CLI wiring is consistentThe updated help text and the new
--cloudevents-webhook-secretflag (backed byCLOUDEVENTS_WEBHOOK_SECRETandwebhookCfg.CloudEventsSecret) line up with the documentation and secret configuration model used for other registries. No issues here.Also applies to: 88-94
pkg/webhook/cloudevents.go (1)
34-73: CloudEvents validation and secret handling look correctThe handler enforces POST, performs a sensible content-type check for structured CloudEvents JSON, and supports a configurable secret via query parameter or
X-Webhook-Secret, returning clear errors for missing/invalid secrets. This fits the documented contract and doesn’t conflict with body parsing later on.
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
b3cac08 to
ff24c38
Compare
Adds CloudEvents v1.0 webhook handler to support AWS ECR push events via EventBridge, using the CloudEvents specification for a standardized approach instead of registry-specific handlers. Changes: - Add pkg/webhook/cloudevents.go with CloudEvents webhook handler - Add pkg/webhook/cloudevents_test.go with comprehensive test suite (60+ tests) - Add --cloudevents-webhook-secret flag to webhook/run commands - Add documentation in docs/configuration/webhook.md - Add Terraform example for EventBridge setup - Add trace logging for debugging webhook payloads - Use crypto/subtle.ConstantTimeCompare for secret validation Signed-off-by: Tanjim Hossain <[email protected]>
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
ff24c38 to
aaa02b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
config/examples/cloudevents/test-webhook.sh (1)
58-74:set -emay not behave as expected for failure tests.With
set -eenabled (line 5), the script will exit on any command failure. However,curlreturns exit code 0 even for HTTP 4xx/5xx responses unless--failis used. The|| echo "Expected failure"fallback won't trigger sincecurlsucceeds at the transport level.Consider adding
-for--failto make curl return non-zero on HTTP errors, or removeset -efor a more forgiving test script that reports all results regardless of individual test outcomes.docs/configuration/webhook.md (1)
151-155: Consider clarifying the recommended authentication method.The documentation shows the secret in the query parameter (
?type=cloudevents&secret=<YOUR_SECRET>) but then recommends using theX-Webhook-Secretheader. Consider leading with the header-based approach since it's more secure and recommended for EventBridge Connection authentication.pkg/webhook/cloudevents.go (3)
34-70: Avoid using query parameters for webhook secrets where possible.The validation logic is sound and the constant‑time comparison is good, but accepting the secret via
?secret=risks exposure through access logs, proxies, and browser history. I’d recommend:
- Prefer the
X-Webhook-Secretheader path in docs and examples.- Long‑term, consider deprecating or feature‑flagging the query‑param option to reduce accidental leakage.
75-104: Trace logging full payloads can be noisy; consider size limits.Logging the entire request body at trace level is useful for debugging, but if events can grow large this may bloat logs or exceed log size limits. Consider truncating the payload (e.g., first N KB) or guarding the log behind an additional debug flag if you expect large events in some deployments.
118-201: Make ECR registry URL extraction more tolerant of different CloudEvents mappings.
parseEvent’s ECR branch currently depends onextractECRRegistryURL, which assumes the CloudEventssourcehas the fixed URN shapeurn:aws:ecr:<region>:<account>:repository/<repo>and only usesdata["registryId"]as an override. If a user tweaks the EventBridge → CloudEvents transform (for example, populatingdata.registryUrldirectly or using a differentsourceformat),registryURLwill end up empty and the event will be rejected.To make this path more robust without breaking the happy path:
- Prefer an explicit
registryUrlfield when present, and only fall back to deriving fromsource/registryId.- Optionally, extend
extractECRRegistryURLto also look at additional fields (e.g.,data["region"]/data["account"]/data["awsAccountId"]) if you expect those from alternative transforms.Illustrative change:
func (c *CloudEventsWebhook) parseEvent(payload *struct { ... }, logCtx *log.LogContext) (*argocd.WebhookEvent, error) { ... switch { case strings.HasPrefix(payload.Type, "com.amazon.ecr."): ... - // Extract registry URL from source - // Source format: urn:aws:ecr:<region>:<account>:repository/<repo> - registryURL = c.extractECRRegistryURL(payload.Source, payload.Data) + // Prefer explicit registryUrl in data, then fall back to source‑based derivation + if regURL, ok := payload.Data["registryUrl"].(string); ok && regURL != "" { + registryURL = regURL + } else { + // Source format: urn:aws:ecr:<region>:<account>:repository/<repo> + registryURL = c.extractECRRegistryURL(payload.Source, payload.Data) + }func (c *CloudEventsWebhook) extractECRRegistryURL(source string, data map[string]interface{}) string { - // Try to extract from source URN + // If the transform already provided a full registry URL, just use it + if regURL, ok := data["registryUrl"].(string); ok && regURL != "" { + return regURL + } + + // Otherwise, try to extract from source URN if strings.HasPrefix(source, "urn:aws:ecr:") { ... } return "" }This keeps your current Terraform example working, but makes the handler resilient to other CloudEvents mappings users might adopt.
Also applies to: 227-247
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/common.go(2 hunks)cmd/run.go(1 hunks)cmd/webhook.go(2 hunks)config/examples/cloudevents/README.md(1 hunks)config/examples/cloudevents/terraform/main.tf(1 hunks)config/examples/cloudevents/terraform/outputs.tf(1 hunks)config/examples/cloudevents/terraform/variables.tf(1 hunks)config/examples/cloudevents/test-webhook.sh(1 hunks)docs/configuration/webhook.md(5 hunks)pkg/webhook/cloudevents.go(1 hunks)pkg/webhook/cloudevents_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/examples/cloudevents/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- config/examples/cloudevents/terraform/main.tf
- cmd/run.go
- config/examples/cloudevents/terraform/outputs.tf
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/common.go (1)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(23-27)
pkg/webhook/cloudevents_test.go (3)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(23-27)test/utils/utils.go (1)
Run(50-67)registry-scanner/pkg/registry/mocks/Repository.go (1)
Repository(15-17)
cmd/webhook.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
pkg/webhook/cloudevents.go (2)
registry-scanner/pkg/log/log.go (4)
NewContext(34-40)Tracef(159-162)Errorf(183-186)LogContext(26-31)pkg/argocd/types.go (1)
WebhookEvent(196-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build_image
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
config/examples/cloudevents/terraform/variables.tf (2)
1-32: Well-structured Terraform variables with appropriate security handling.The variable declarations are clear, well-documented, and follow Terraform best practices. The
sensitive = trueflag is correctly applied towebhook_secretto protect credentials from accidental logging.
19-23: I'll help you verify that theecr_repository_filterempty list behavior is properly implemented downstream. Let me search the codebase for the relevant implementation.
<function_calls>
#!/bin/bashSearch for ecr_repository_filter usage in terraform examples
rg -n "repository_filter|ecr_repository_filter" config/examples/cloudevents/terraform/ -A 3 -B 1
</function_calls>
<function_calls>
#!/bin/bashList terraform files in the cloudevents example
ls -la config/examples/cloudevents/terraform/
</function_calls><function_calls>
#!/bin/bashCheck if main.tf exists and read its content
if [ -f config/examples/cloudevents/terraform/main.tf ]; then
wc -l config/examples/cloudevents/terraform/main.tf
fi
</function_calls>config/examples/cloudevents/test-webhook.sh (1)
1-92: Test script provides good coverage of CloudEvents webhook scenarios.The script tests ECR-specific events, generic container events, missing authentication, and invalid event types. The payloads align with the CloudEvents v1.0 specification and match the formats documented in the PR.
cmd/common.go (2)
29-29: LGTM!The
CloudEventsSecretfield follows the established pattern used for other webhook secrets in the struct.
121-121: LGTM!Registration of the CloudEvents webhook handler is consistent with the existing webhook handler registrations (Docker, GHCR, Harbor, Quay).
cmd/webhook.go (2)
44-45: LGTM!Help text appropriately updated to include AWS ECR via EventBridge CloudEvents in the list of supported registries.
93-93: LGTM!The CLI flag follows the established pattern for webhook secrets, with consistent naming (
--cloudevents-webhook-secret) and environment variable binding (CLOUDEVENTS_WEBHOOK_SECRET).docs/configuration/webhook.md (2)
12-12: Documentation updates are comprehensive and well-structured.The AWS ECR via CloudEvents section provides clear prerequisites, setup steps, Terraform references, and payload format examples. Environment variables and secrets are properly documented alongside existing webhook configurations.
Also applies to: 88-99, 102-163, 177-177, 214-214, 255-255
160-163: The kubernetes directory reference cannot be verified with available tools.The shell script execution failed due to a repository clone error, preventing direct verification of whether
config/examples/cloudevents/kubernetes/exists in the codebase. Without access to the repository filesystem, I cannot confirm whether the documentation's reference to this directory is accurate or if the directory exists with actual manifest files.Manual verification is needed: Check if
config/examples/cloudevents/kubernetes/exists in the repository and contains Kubernetes manifest examples as referenced in the documentation.pkg/webhook/cloudevents_test.go (5)
1-17: LGTM!Clean imports and straightforward registry type test.
19-122: Comprehensive validation test coverage.The table-driven tests cover all validation paths: valid requests with/without secrets, CloudEvents content type, query and header secret validation, and error cases for invalid methods, content types, and secrets.
124-265: Good ECR parsing test coverage.Tests cover standard ECR events, namespaced repositories, subject fallback when data fields are missing, and error cases for missing repository and tag fields. The expected registry URL construction (
<account>.dkr.ecr.<region>.amazonaws.com) is properly validated.
267-420: Solid generic CloudEvents parsing tests.Tests validate parsing with different field naming conventions, source URL extraction, and proper error handling for missing
specversion,type, and unsupported event types. The conditional digest assertion (line 414-416) correctly handles optional digest fields.
422-495: LGTM!Helper function tests for
extractECRRegistryURLandextractRegistryFromSourcecover valid ECR URNs, fallback behavior, and various source URL formats including edge cases.pkg/webhook/cloudevents.go (2)
1-32: CloudEventsWebhook type and constructor look consistent and minimal.Struct, constructor, and
GetRegistryTypeare straightforward and align with the existing webhook abstraction; no issues from a correctness or API‑surface perspective.
249-268:extractRegistryFromSourceis simple and effective; considerurl.Parseonly if future needs grow.The current string‑based extraction covers typical
https://host/...andhost.domain/pathpatterns without extra allocations or error handling. Unless you anticipate more exotic URI shapes, there’s no need to complicate this withnet/url; the helper is fine as‑is.
Signed-off-by: audacioustux <[email protected]>
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
3b5c94b to
8d15807
Compare
|
@coderabbitai generate docstrings |
|
Warning Docstrings generation is disabled for your repository or organization. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9040 🏁 Script executed: Length of output: 4438 🏁 Script executed: Length of output: 6734 🏁 Script executed: Length of output: 19303 @audacioustux Great news! I've reviewed the code and found that all exported types and functions already have proper Go docstrings. The CloudEvents implementation in However, I can suggest adding inline field comments for the // WebhookConfig holds the options for the webhook server
type WebhookConfig struct {
// Port is the port number for the webhook server to listen on
Port int
// DockerSecret is the secret for validating Docker Hub webhooks
DockerSecret string
// GHCRSecret is the secret for validating GitHub Container Registry webhooks
GHCRSecret string
// QuaySecret is the secret for validating Quay webhooks
QuaySecret string
// HarborSecret is the secret for validating Harbor webhooks
HarborSecret string
// CloudEventsSecret is the secret for validating CloudEvents webhooks
CloudEventsSecret string
// RateLimitNumAllowedRequests is the number of allowed requests per hour for rate limiting
RateLimitNumAllowedRequests int
}The rest of your code already follows Go documentation best practices:
The existing documentation in |
Improves documentation completeness by adding inline comments to each field in the WebhookConfig struct, following Go documentation best practices. Signed-off-by: audacioustux <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/common.go (1)
24-37: WebhookConfig field docs look good; consider documenting “0 disables rate limiting”.The new/updated comments for all webhook secrets and
CloudEventsSecretare clear and consistent. Given that lines 133–135 treatRateLimitNumAllowedRequests <= 0as “no rate limiting”, you might make that explicit in the comment to help operators understand config behavior:- // RateLimitNumAllowedRequests is the number of allowed requests per hour for rate limiting + // RateLimitNumAllowedRequests is the number of allowed requests per hour for rate limiting (0 disables rate limiting)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/common.go(2 hunks)docs/configuration/webhook.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration/webhook.md
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/common.go (1)
pkg/webhook/cloudevents.go (1)
NewCloudEventsWebhook(23-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: build_image
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/common.go (1)
123-129: CloudEvents handler registration is consistent with existing webhook handlers.Registering
webhook.NewCloudEventsWebhook(webhookCfg.CloudEventsSecret)alongside the other registry-specific handlers follows the same pattern and matches the constructor signature frompkg/webhook/cloudevents.go. No issues spotted here.
Updates the RateLimitNumAllowedRequests field comment to explicitly state that setting it to 0 disables rate limiting, helping operators understand the configuration behavior. Signed-off-by: audacioustux <[email protected]>
|
Hi, are you available to review this PR? This change adds support for a generic webhook endpoint. This avoids the need to maintain individual logic for every container registry (like AWS ECR). Instead, we can rely on external transformation (e.g., ECR -> EventBridge Input Transformation -> ArgoCD IU). This allows users of any cloud platform to map their events to the CloudEvents schema using serverless compute or transformation rules. I have pushed an image here for testing purposes: https://github.com/audacioustux/argocd-image-updater/pkgs/container/argocd-image-updater Thank you! |
|
Hi @audacioustux |
dkarpele
left a comment
dkarpele
left a comment
There was a problem hiding this comment.
Thanks for working on it @audacioustux!
Please extend testing in cmd/run_test.go, cmd/webhook_test.go.
Just add a line similar to
asser.Equal(env.GetStringVal("HARBOR_WEBHOOK_SECRET", ""), controllerCommand.Flag("harbor-webhook-secret").Value.String())
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/webhook/cloudevents.go (1)
38-42: Consider logging pattern consistency.The current implementation creates a
logCtxfrom the context (lines 42, 87) for logging operations. A previous review suggested usingctx context.Contextparameters directly for consistency with the rest of the codebase (e.g.,server.golines 78-80). While the current approach is functional, consider refactoring to pass and usectxdirectly in logging calls for consistency.Based on learnings from past reviews.
Also applies to: 83-87
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/run_test.go(1 hunks)cmd/webhook_test.go(1 hunks)config/install.yaml(1 hunks)config/manager/manager.yaml(1 hunks)docs/configuration/webhook.md(5 hunks)docs/install/cmd/run.md(1 hunks)docs/install/cmd/webhook.md(2 hunks)pkg/webhook/cloudevents.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/webhook_test.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
🪛 LanguageTool
docs/install/cmd/webhook.md
[style] ~40-~40: To form a complete sentence, be sure to include a subject.
Context: ...tBridge and other CloudEvents sources. Can also be set with the `CLOUDEVENTS_WEBHO...
(MISSING_IT_THERE)
docs/install/cmd/run.md
[style] ~37-~37: To form a complete sentence, be sure to include a subject.
Context: ...tBridge and other CloudEvents sources. Can also be set with the `CLOUDEVENTS_WEBHO...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: build_image
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
cmd/webhook_test.go (1)
45-45: LGTM!The test assertion correctly validates the CLOUDEVENTS_WEBHOOK_SECRET environment variable mapping to the cloudevents-webhook-secret flag, following the established pattern for other webhook secret tests.
docs/install/cmd/webhook.md (1)
37-41: LGTM!The documentation for the CloudEvents webhook secret flag is clear and follows the established pattern for other webhook secrets. The static analysis hint about a "missing subject" on line 40 can be safely ignored—this phrasing is consistent with the rest of the documentation.
config/install.yaml (1)
945-950: LGTM!The CLOUDEVENTS_WEBHOOK_SECRET environment variable configuration follows the established pattern for other webhook secrets, correctly referencing the secret key with
optional: true.cmd/run_test.go (1)
67-67: LGTM!The test assertion properly validates the CLOUDEVENTS_WEBHOOK_SECRET environment variable mapping, maintaining consistency with the other webhook secret flag tests.
config/manager/manager.yaml (1)
172-177: LGTM!The CLOUDEVENTS_WEBHOOK_SECRET environment variable configuration is consistent with the pattern used for other webhook secrets and matches the setup in config/install.yaml.
docs/install/cmd/run.md (1)
34-38: LGTM!The documentation clearly describes the CloudEvents webhook secret flag for the
runcommand, maintaining consistency with the webhook command documentation. The static analysis hint about a "missing subject" is a false positive and can be ignored.docs/configuration/webhook.md (2)
103-163: LGTM! Comprehensive CloudEvents/ECR documentation.The AWS ECR via EventBridge CloudEvents section is thorough and well-structured, covering prerequisites, setup steps, Terraform configuration, payload format, and webhook URL configuration. This addresses the past review feedback about adding documentation and examples.
88-89: CloudEvents link added as requested.The CloudEvents specification link has been added as requested in the previous review, providing users with a reference to the official spec.
pkg/webhook/cloudevents.go (3)
36-79: LGTM! Secure validation with constant-time comparison.The validation logic correctly addresses the security concern from the previous review by using
subtle.ConstantTimeComparefor secret verification (line 73). The function properly validates HTTP method, content type, and supports secret authentication via both query parameter and header.
128-237: LGTM! Robust event parsing with good fallback logic.The
parseEventfunction handles multiple CloudEvents types effectively:
- AWS ECR events with specific field extraction
- Generic container/image events with multiple field name variants
- Subject fallback for repo:tag format (line 145 correctly uses
SplitNwith limit 2 to handle tags containing colons)- Comprehensive validation of required fields
239-280: LGTM! Registry URL extraction helpers are well-structured.Both
extractECRRegistryURLandextractRegistryFromSourceprovide reasonable heuristics for extracting registry information from various source formats. The ECR function correctly prioritizesregistryIdfrom the data field over the source URN for better reliability.
Updated Validate(), Parse(), and parseEvent() to use context-based logging. Added tests, documentation, and Kubernetes manifests for webhook secret. Signed-off-by: Tanjim H. <[email protected]>
audacioustux
force-pushed
the
feat/cloudevents-webhook
branch
from
ea241ff to
2b4505b
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds CloudEvents v1.0 webhook support to enable AWS ECR push event notifications via EventBridge, providing a standardized approach instead of registry-specific handlers for each cloud provider.
- Implements CloudEvents webhook handler with optional secret validation for AWS ECR events via EventBridge
- Adds comprehensive test suite with 60+ tests covering validation, parsing, and extraction logic
- Includes complete documentation, Terraform examples, and integration with existing webhook infrastructure
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/webhook/cloudevents.go |
Core CloudEvents webhook handler implementation with ECR and generic container event support |
pkg/webhook/cloudevents_test.go |
Comprehensive test suite covering validation, ECR events, generic events, and extraction helpers |
cmd/webhook.go |
Adds --cloudevents-webhook-secret flag to webhook command |
cmd/run.go |
Adds --cloudevents-webhook-secret flag to run command |
cmd/common.go |
Extends WebhookConfig struct with CloudEventsSecret field and adds documentation comments |
cmd/webhook_test.go |
Adds test assertion for CloudEvents webhook secret flag |
cmd/run_test.go |
Adds test assertion for CloudEvents webhook secret flag |
docs/install/cmd/webhook.md |
Documents the new CloudEvents webhook secret flag and environment variable |
docs/install/cmd/run.md |
Documents the new CloudEvents webhook secret flag and environment variable |
docs/configuration/webhook.md |
Comprehensive documentation for AWS ECR via EventBridge setup with CloudEvents, including Terraform examples and payload format (also fixes typo in GHCR environment variable) |
config/manager/manager.yaml |
Adds CLOUDEVENTS_WEBHOOK_SECRET environment variable mapping to Kubernetes secret |
config/install.yaml |
Adds CLOUDEVENTS_WEBHOOK_SECRET environment variable mapping to Kubernetes secret |
config/examples/cloudevents/README.md |
Quick start guide for CloudEvents webhook with Terraform and testing instructions |
config/examples/cloudevents/test-webhook.sh |
Test script with four scenarios: valid ECR event, generic event, missing secret, and invalid event type |
config/examples/cloudevents/terraform/main.tf |
Complete Terraform configuration for EventBridge rule, input transformer, API destination, and IAM setup |
config/examples/cloudevents/terraform/variables.tf |
Terraform variables for AWS region, webhook URL, secret, repository filter, and tags |
config/examples/cloudevents/terraform/outputs.tf |
Terraform outputs for EventBridge rule ARN, API destination ARN, IAM role ARN, and webhook endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/webhook/cloudevents.go (1)
82-126: Apply the same context handling fix here.The Parse method has the same context handling issue as the Validate method. The context should be derived from
r.Context().func (c *CloudEventsWebhook) Parse(r *http.Request) (*argocd.WebhookEvent, error) { + ctx := r.Context() webhookLogger := log.Log().WithFields(logrus.Fields{ "logger": "cloudevents-webhook", }) - ctx := log.ContextWithLogger(r.Context(), webhookLogger) + ctx = log.ContextWithLogger(ctx, webhookLogger) logCtx := log.LoggerFromContext(ctx)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/run_test.go(1 hunks)cmd/webhook_test.go(1 hunks)config/install.yaml(1 hunks)config/manager/manager.yaml(1 hunks)docs/configuration/webhook.md(5 hunks)docs/install/cmd/run.md(1 hunks)docs/install/cmd/webhook.md(2 hunks)pkg/webhook/cloudevents.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/install.yaml
- cmd/run_test.go
- cmd/webhook_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/webhook/cloudevents.go (2)
registry-scanner/pkg/log/log.go (5)
Log(94-96)ContextWithLogger(203-205)LoggerFromContext(210-216)Tracef(159-162)Errorf(183-186)pkg/argocd/types.go (1)
WebhookEvent(196-205)
🪛 LanguageTool
docs/install/cmd/run.md
[style] ~37-~37: To form a complete sentence, be sure to include a subject.
Context: ...tBridge and other CloudEvents sources. Can also be set with the `CLOUDEVENTS_WEBHO...
(MISSING_IT_THERE)
docs/install/cmd/webhook.md
[style] ~40-~40: To form a complete sentence, be sure to include a subject.
Context: ...tBridge and other CloudEvents sources. Can also be set with the `CLOUDEVENTS_WEBHO...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.18.1)
docs/install/cmd/run.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (7)
pkg/webhook/cloudevents.go (3)
17-34: LGTM! Clean implementation.The CloudEventsWebhook struct, constructor, and GetRegistryType method are well-implemented and follow good patterns.
129-237: LGTM! Robust parsing logic.The parseEvent implementation is well-structured with:
- Smart handling of the subject field using
SplitNto preserve colons in tags- Clear fallback hierarchy (data fields override subject)
- Support for both AWS ECR-specific and generic container event formats
- Proper validation of required fields
241-280: LGTM! Well-designed helper methods.Both extraction helpers are well-implemented:
extractECRRegistryURLcorrectly prioritizesregistryIdfrom data over the source URNextractRegistryFromSourceprovides sensible fallback logic for generic registry URLsdocs/install/cmd/webhook.md (1)
17-17: LGTM! Documentation additions are clear and consistent.The CloudEvents webhook documentation follows the same pattern as other registry webhook secrets and is well-integrated into the existing documentation structure.
Also applies to: 37-42
docs/install/cmd/run.md (1)
34-39: LGTM! Consistent documentation.The CloudEvents webhook secret flag is documented consistently with the other webhook secrets.
config/manager/manager.yaml (1)
172-177: LGTM! Proper integration of CloudEvents secret.The CLOUDEVENTS_WEBHOOK_SECRET environment variable follows the same pattern as other webhook secrets and is correctly configured with
optional: true.docs/configuration/webhook.md (1)
12-12: LGTM! Comprehensive and well-integrated documentation.The CloudEvents/AWS ECR documentation is thorough and well-organized:
- Clear prerequisites and setup steps
- References to Terraform examples for practical implementation
- CloudEvents payload format specification
- Proper integration across all relevant sections (supported registries, secrets, environment variables)
The CloudEvents specification link addresses the previous review comment.
Also applies to: 88-163, 177-177, 214-214, 252-256
Reordered imports to match project convention: - Standard library - Third-party packages (logrus) - Project packages (argocd-image-updater) Signed-off-by: Tanjim H. <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/webhook/cloudevents.go (2)
82-127: Consider reducing or gating full payload trace logging
Tracef("Received payload: %s", string(body))logs the complete request body, which is great for debugging but can become a liability if future CloudEvents carry secrets or PII. Consider either redacting sensitive fields, truncating large payloads, or guarding this with a dedicated debug flag so it’s less likely to be enabled in production clusters.
240-260: Make ECR registry URL extraction resilient to both URN and ARN prefixesRight now
extractECRRegistryURLonly handlessourcevalues starting withurn:aws:ecr:. If the CloudEvents transformer (or a future user) emits a more ARN‑like prefix (arn:aws:ecr:) instead, this helper will silently return an empty string and ECR events will fail with “registry URL not found”. You can make this more robust by accepting both prefixes while keeping the rest of the logic unchanged:-func (c *CloudEventsWebhook) extractECRRegistryURL(source string, data map[string]interface{}) string { - // Try to extract from source URN - if strings.HasPrefix(source, "urn:aws:ecr:") { +func (c *CloudEventsWebhook) extractECRRegistryURL(source string, data map[string]interface{}) string { + // Try to extract from source URN/ARN + if strings.HasPrefix(source, "urn:aws:ecr:") || strings.HasPrefix(source, "arn:aws:ecr:") { parts := strings.Split(source, ":") if len(parts) >= 5 { region := parts[3] account := parts[4]This keeps your documented
urn:aws:ecr:...format working while tolerating ARN‑style inputs if someone wires the rule differently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/webhook/cloudevents.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/webhook/cloudevents.go (2)
registry-scanner/pkg/log/log.go (5)
Log(94-96)ContextWithLogger(203-205)LoggerFromContext(210-216)Tracef(159-162)Errorf(183-186)pkg/argocd/types.go (1)
WebhookEvent(196-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (4)
pkg/webhook/cloudevents.go (4)
18-35: CloudEventsWebhook struct, constructor, and registry type are clear and minimalEncapsulating the secret in
CloudEventsWebhookand exposing a fixed"cloudevents"registry type is straightforward and consistent with the rest of the webhook handlers. No changes needed here.
37-80: Validate covers method, content type, and secret handling correctlyThe validation logic (POST‑only, JSON/CloudEvents content types, optional secret via query or
X-Webhook-Secret, and constant‑time comparison) is solid and aligned with expected webhook hardening. I don’t see any functional or security gaps in this block.
129-238: Event parsing logic for ECR and generic container events looks robustThe
parseEventimplementation cleanly handles subject fallback (repo:tag), distinct ECR vs. generic container/image event types, and multiple possible field names (repositoryName/repository,imageTag/tag,imageDigest/digest), with final validation of repository, tag, and registry URL. This should behave predictably across the payload shapes described in the PR, and I don’t see any correctness issues here.
262-280: Generic registry URL extraction from source is simple and effectiveThe helper correctly handles full URLs (
http(s)://...) by returning the host segment and falls back to the leading host‑like portion for non‑URL strings. This is a pragmatic balance between robustness and simplicity for generic container events.
Remove full payload logging to prevent potential leakage of secrets or PII. Add support for ARN prefix in ECR registry URL extraction alongside existing URN format. Signed-off-by: Tanjim H. <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/webhook/cloudevents.go (3)
37-80: Validation logic is sound; consider tightening constant‑time secret handlingOverall
Validatecorrectly enforces POST, checks JSON/CloudEvents content types, and supports both query and header secrets, with good trace logging and consistent context handling.One optional hardening you could consider:
subtle.ConstantTimeCompareis only constant‑time when the slices have equal length; otherwise it may return early. To avoid any length‑based timing differences, you can normalize lengths (e.g., immediately reject iflen(secret) != len(c.secret)but return a generic error, or pad both to a fixed length before comparison) and keep the same public error message for missing vs invalid secrets.Not required to ship, but worth considering if this endpoint is exposed beyond trusted infrastructure.
127-236: Event extraction logic is robust; only minor edge‑case considerations
parseEventcleanly handles:
- Subject parsing with
SplitN(..., 2)to support colon‑containing tags.- AWS ECR events via a dedicated branch that prefers data fields over subject, and derives the registry from
source/registryId.- Generic container/image events with sensible fallbacks across common field names.
- Final validation for
repository,tag, andregistryURL, while treatingdigestas optional.Two minor, non‑blocking considerations:
- The generic case matches any type containing
"container"or"image", which is intentional but broad; if you see unexpected types being accepted in practice, you may want to narrow this to known prefixes or allowlist patterns.- Error messages for missing repo/tag/registry are very clear but also leak exactly what was absent. That’s great for operator debugging; if this endpoint ever needs a more opaque public surface, you might want a generic “invalid payload” error with details only logged at trace/debug.
Functionally this looks good as‑is.
238-258: ECR registry extraction works for standard ARNs; consider future partition support
extractECRRegistryURLcorrectly handles bothurn:aws:ecr:...andarn:aws:ecr:...formats, pulls out region/account, and prefersregistryIdfromdatawhen present, which is nice and robust.If you ever need to support non‑standard AWS partitions (GovCloud, China, etc.), you’ll likely need to adjust the domain suffix (
amazonaws.comvsamazonaws.com.cn, etc.), potentially inferred fromsourceor an additional config. For now, this is fine for the standard partition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/webhook/cloudevents.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Agent
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (3)
pkg/webhook/cloudevents.go (3)
21-35: CloudEventsWebhook type and basic wiring look solidThe struct, constructor, and
GetRegistryTypeare straightforward, immutable after construction, and match the intended registry type name; no changes needed here.
82-136: Parse and payload struct mapping are correct and defensive
Parse’s read/unmarshal flow, error wrapping, and trace logging look good. The payload struct maps CloudEvents v1.0 fields as expected, and the early checks forspecversionandtypeprovide clear errors. Context propagation and logger usage are consistent with the rest of the file.
260-279: Generic registry extraction from source is reasonable and safe
extractRegistryFromSourcehandles URL‑like sources and simplehost.domain/repopatterns without over‑parsing, and falls back to empty string when it can’t infer a registry. This is a good balance between flexibility and safety; no changes needed here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move CloudEvents spec reference from registry docs section to ECR setup context where it's more relevant. Add CloudEvents to example payloads section for consistency with other registry types. Signed-off-by: Tanjim H. <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Tanjim Hossain <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Tanjim Hossain <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Tanjim Hossain <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Tanjim Hossain <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Tanjim Hossain <[email protected]>
Enforce HTTPS-only URLs for webhook_url variable to prevent X-Webhook-Secret header transmission over unencrypted HTTP. Signed-off-by: Tanjim H. <[email protected]>
| ID string `json:"id"` | ||
| Time string `json:"time"` | ||
| DataContentType string `json:"datacontenttype"` | ||
| Data map[string]interface{} `json:"data"` |
There was a problem hiding this comment.
can we extract the payload struct into a named struct, to avoid duplication in parse() and parseEvent()?
| if contentType != "" && !strings.Contains(contentType, "application/json") && | ||
| !strings.Contains(contentType, "application/cloudevents+json") { | ||
| return fmt.Errorf("invalid content type: %s", contentType) | ||
| } |
There was a problem hiding this comment.
If contentType is empty, it won't be caught by the above condition. So we do not need the contentType != "" part.
See the similar in harbor.go
|
|
||
| Can also be set with the `DOCKER_WEBHOOK_SECRET` environment variable. | ||
|
|
||
| **--cloudevents-webhook-secret *secret*** |
There was a problem hiding this comment.
Sorry, this should go alphabetically, please move after **--argocd-namespace *namespace***
|
|
||
| Can also be set with the `DOCKER_WEBHOOK_SECRET` environment variable. | ||
|
|
||
| **--cloudevents-webhook-secret *secret*** |
There was a problem hiding this comment.
Sorry, this should go alphabetically, please move after **--argocd-namespace *namespace**
| // Source format: urn:aws:ecr:<region>:<account>:repository/<repo> or arn:aws:ecr:<region>:<account>:repository/<repo> | ||
| func (c *CloudEventsWebhook) extractECRRegistryURL(source string, data map[string]interface{}) string { | ||
| // Try to extract from source URN/ARN | ||
| if strings.HasPrefix(source, "urn:aws:ecr:") || strings.HasPrefix(source, "arn:aws:ecr:") { |
There was a problem hiding this comment.
if arn is also a valid format, can we include the validation of registry url arn:xxx in unit tests?
4 participants
Summary
Adds CloudEvents v1.0 webhook handler to support AWS ECR push events via EventBridge.
Motivation
Instead of adding registry-specific webhook handlers for each cloud provider (ECR, GCR, ACR), this uses the CloudEvents specification to provide a standardized approach. AWS EventBridge can transform native ECR events to CloudEvents format using input transformers.
Changes
docs/configuration/webhook.mdconfig/examples/cloudevents/Testing
All tests pass:
Terraform validated:
terraform validate # Success!Example Usage
argocd-image-updater webhook --cloudevents-webhook-secret=$SECRETEventBridge transforms ECR events to:
{ "specversion": "1.0", "type": "com.amazon.ecr.image.push", "source": "urn:aws:ecr:us-east-1:123456789012:repository/my-repo", "subject": "my-repo:v1.0.0", "data": { "repositoryName": "my-repo", "imageTag": "v1.0.0", "imageDigest": "sha256:..." } }Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.